Automated Test: workflow-queue-enhanced #373
Conversation
…re still sent (#7232) * small UI fix * fix cancelling scheduled emails * improve comments * delete reminders for rescheduled bookings * add migration file * cancel rescheduled bookings immediately * remove immediate delete for request reschedule --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
📝 WalkthroughWalkthroughThis PR refactors the workflow reminder cancellation system to use direct function calls instead of batch Prisma operations, introduces a "cancelled" database field to track cancellation state, adds automated cancellation for pending email reminders, updates phone number verification UI with improved verification flow, and enhances booking payload construction with language/locale information across multiple modules. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as Email Scheduler<br/>(scheduleEmailReminders)
participant DB as Prisma DB
participant API as External API<br/>(Brevo/ESP)
Scheduler->>DB: Query cancelled reminders<br/>(scheduled within 1 hour)
DB-->>Scheduler: Return cancelled reminders list
loop For each cancelled reminder
Scheduler->>API: POST /v3/user/scheduled_sends<br/>(batch_id, status: "cancel")
API-->>Scheduler: Cancellation response
end
Scheduler->>DB: Batch delete cancelled reminders<br/>(Promise.all)
DB-->>Scheduler: Deletion confirmed
Scheduler->>Scheduler: Log success/errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@packages/features/bookings/lib/handleCancelBooking.ts`:
- Around line 487-491: The cancellation path currently calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId) (and
deleteScheduledSMSReminder(...)) without the immediateDelete flag so reminders
are merely marked cancelled and may still send; update the calls inside the
if/else that checks reminder.method (the block using WorkflowMethods.EMAIL and
WorkflowMethods.SMS) to pass immediateDelete: true as the third argument (e.g.,
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) and
deleteScheduledSMSReminder(reminder.id, reminder.referenceId, true)) so the
reminders are immediately cancelled and removed from SendGrid/SMS providers.
In `@packages/features/bookings/lib/handleNewBooking.ts`:
- Around line 963-975: The current loop over
originalRescheduledBooking.workflowReminders calls async functions
deleteScheduledEmailReminder and deleteScheduledSMSReminder inside a forEach
without awaiting, so errors escape the try-catch; modify the block in
handleNewBooking where originalRescheduledBooking?.uid is checked to collect the
promises (e.g., map or push to an array) for each reminder depending on
reminder.method === WorkflowMethods.EMAIL or SMS, then await
Promise.all(promises) inside the existing try so any rejection is caught by the
catch; keep the existing guards out since the called functions already handle
null referenceId.
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts`:
- Around line 43-51: The cancellation query in scheduleEmailReminders.ts is
missing a method filter and may return non-email reminders; update the
prisma.workflowReminder.findMany call that produces remindersToCancel to include
method: WorkflowMethods.EMAIL (alongside the existing cancelled and
scheduledDate filters) so only email reminders are selected; reference
WorkflowMethods.EMAIL, remindersToCancel, and prisma.workflowReminder.findMany
when making this change.
- Around line 53-77: The current try/catch around the whole loop causes a single
failure to abort processing remaining reminders; change to per-iteration error
isolation by moving the try/catch inside the for loop so each reminder is
processed independently: for each reminder in remindersToCancel call
client.request(...) and prepare prisma.workflowReminder.delete(...) inside its
own try block, push successful deletes to workflowRemindersToDelete, and
handle/log errors for that specific reminder (include reminder.id or
referenceId) without breaking the loop, then await
Promise.all(workflowRemindersToDelete) after the loop; symbols to modify:
remindersToCancel, client.request, prisma.workflowReminder.delete,
workflowRemindersToDelete.
- Around line 56-64: The loop over remindersToCancel calls client.request to
POST to "/v3/user/scheduled_sends" using reminder.referenceId as batch_id
without checking for null; add a guard inside the for (const reminder of
remindersToCancel) loop that skips (or logs and continues) when
reminder.referenceId is null/undefined before calling client.request so we never
send a null batch_id to the SendGrid scheduled_sends endpoint; reference the
reminder.referenceId check and the client.request(...) call in your change.
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx`:
- Around line 436-458: The placeholder "Verification code" on the TextField and
the button label "Verify" are hardcoded; update WorkflowStepContainer to call
the translation function t(...) for these strings (replace the TextField
placeholder and the Button children with t('verification_code') and t('verify')
respectively), keeping the existing verificationCode state and
verifyPhoneNumberMutation logic intact; then add matching keys
"verification_code" and "verify" to your locale JSON files (e.g., common.json)
for all supported locales or reuse appropriate existing keys.
In `@packages/features/ee/workflows/lib/reminders/emailReminderManager.ts`:
- Around line 213-223: When immediateDelete is true the code only cancels the
SendGrid scheduled send via client.request but never removes the
WorkflowReminder DB record; update the immediateDelete branch in
emailReminderManager (where client.request is called) to also delete the
corresponding reminder record from the database (mirror the behavior in
deleteScheduledSMSReminder and the cron in scheduleEmailReminders.ts) — locate
the reminder by referenceId or the function's reminder id parameter and call the
same DB delete routine used elsewhere so the WorkflowReminder row is removed
after cancelling with SendGrid.
In `@packages/trpc/server/routers/viewer/bookings.tsx`:
- Around line 487-494: The email reminder cancellation in the reschedule path
omits the immediateDelete flag so deleteScheduledEmailReminder(reminder.id,
reminder.referenceId) should be called with immediateDelete=true like in
cancellation flows; update the loop handling
bookingToReschedule.workflowReminders to pass the third argument true for email
reminders (and leave deleteScheduledSMSReminder(reminder.id,
reminder.referenceId) unchanged) so previous booking emails are removed
immediately.
In `@packages/trpc/server/routers/viewer/workflows.tsx`:
- Around line 211-218: scheduledReminders are being deleted with fire-and-forget
for deleteScheduledEmailReminder and deleteScheduledSMSReminder; change each
removal block (the one iterating over scheduledReminders and the other similar
blocks) to first filter out reminders with null referenceId, map to the async
deletion calls (using WorkflowMethods.EMAIL/SMS to choose
deleteScheduledEmailReminder/deleteScheduledSMSReminder), collect the returned
promises, and await them with Promise.all() so deletions are awaited and
rejections handled.
🧹 Nitpick comments (5)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (3)
408-421: Add loading state to prevent duplicate requests.The "Send Code" button lacks a loading/disabled state while
sendVerificationCodeMutationis in flight. This could allow users to spam the button and trigger multiple verification code sends.⏳ Proposed fix to add loading state
<Button color="secondary" - disabled={numberVerified || false} + disabled={numberVerified || sendVerificationCodeMutation.isLoading} className={classNames( "-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ", numberVerified ? "hidden" : "mt-3 sm:mt-0" )} onClick={() => sendVerificationCodeMutation.mutate({ phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", }) }> - {t("send_code")} + {sendVerificationCodeMutation.isLoading ? t("sending") : t("send_code")} </Button>
437-445: Add accessible label for verification code input.The verification code
TextFieldrelies solely on a placeholder for context. Consider adding alabelprop or wrapping with aLabelcomponent for screen reader accessibility.♿ Proposed fix for accessibility
<TextField className=" border-r-transparent" - placeholder="Verification code" + placeholder={t("verification_code")} + label={t("verification_code")} + labelSrOnly value={verificationCode} onChange={(e) => { setVerificationCode(e.target.value); }} required />
155-168: Clear verification code state after successful verification.After successful verification,
verificationCoderemains populated. While the UI hides the input, clearing state maintains cleaner component state management.🧹 Proposed fix to clear state
const verifyPhoneNumberMutation = trpc.viewer.workflows.verifyPhoneNumber.useMutation({ onSuccess: async (isVerified) => { showToast(isVerified ? t("verified_successfully") : t("wrong_code"), "success"); setNumberVerified(isVerified); + if (isVerified) { + setVerificationCode(""); + } utils.viewer.workflows.getVerifiedNumbers.invalidate(); },packages/features/bookings/lib/handleCancelBooking.ts (1)
484-493: Fire-and-forget pattern may silently lose cancellation failures.The
deleteScheduledEmailReminderanddeleteScheduledSMSRemindercalls are not awaited and their results are not collected. While these functions have internal try/catch blocks that log errors, the calling code has no way to know if cancellations failed. For booking cancellation, this could result in reminders being sent for cancelled bookings without any indication of the failure.Consider collecting the promises and awaiting them, or at minimum logging at this level if failures occur.
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-235: Consider documenting the three distinct cancellation paths.The function now has three different behaviors based on the parameters, which may not be immediately clear to callers:
referenceIdis null → Delete from DB only (reminder was never scheduled with SendGrid)immediateDeleteis true → Cancel with SendGrid API (and should delete from DB per the fix above)- Otherwise → Mark as
cancelled: truefor deferred processing by cronA brief JSDoc comment would help future maintainers understand when to use each path.
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } |
There was a problem hiding this comment.
Missing immediateDelete flag may cause inconsistent cancellation behavior.
When a booking is cancelled, deleteScheduledEmailReminder is called without the immediateDelete flag (third parameter). Looking at the implementation, this means the function will only set cancelled: true on the reminder record instead of immediately cancelling it with SendGrid and deleting the record.
In contrast, handleNewBooking.ts (for reschedules) passes immediateDelete: true:
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);For a cancelled booking, immediate cancellation is likely desired to ensure the reminder doesn't get sent. The deferred path relies on a cron job that runs hourly, which could allow a reminder to be sent if the booking is cancelled close to the scheduled time.
🐛 Proposed fix to add immediateDelete flag
booking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 487 -
491, The cancellation path currently calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId) (and
deleteScheduledSMSReminder(...)) without the immediateDelete flag so reminders
are merely marked cancelled and may still send; update the calls inside the
if/else that checks reminder.method (the block using WorkflowMethods.EMAIL and
WorkflowMethods.SMS) to pass immediateDelete: true as the third argument (e.g.,
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) and
deleteScheduledSMSReminder(reminder.id, reminder.referenceId, true)) so the
reminders are immediately cancelled and removed from SendGrid/SMS providers.
| if (originalRescheduledBooking?.uid) { | ||
| try { | ||
| // cancel workflow reminders from previous rescheduled booking | ||
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find implementations of deleteScheduled* functions
rg -n "deleteScheduledEmailReminder|deleteScheduledSMSReminder" packages -g '*.ts' -A 10 | head -100Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 8028
🏁 Script executed:
#!/bin/bash
# Search for where these functions are defined
rg -n "export.*deleteScheduledEmailReminder|export.*deleteScheduledSMSReminder|function deleteScheduledEmailReminder|function deleteScheduledSMSReminder" packages -g '*.ts'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Look for the reminders directory structure
fd -t f -name "*.ts" packages/features/ee/workflows | grep -i reminder | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 320
🏁 Script executed:
#!/bin/bash
cat packages/features/ee/workflows/lib/reminders/emailReminderManager.ts | sed -n '197,230p'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 761
🏁 Script executed:
#!/bin/bash
cat packages/features/ee/workflows/lib/reminders/smsReminderManager.ts | sed -n '177,200p'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 462
Await reminder deletions to ensure proper error handling.
The deleteScheduledEmailReminder and deleteScheduledSMSReminder functions are async, but the current code calls them in a forEach without awaiting, creating fire-and-forget promises. Any errors thrown within these functions won't be caught by the outer try-catch block. Both functions safely handle null referenceId with internal guards (email checks if (!referenceId), SMS checks if (referenceId)), so filtering is unnecessary. Instead, collect the promises and await them all:
🛠️ Proposed fix
- try {
- // cancel workflow reminders from previous rescheduled booking
- originalRescheduledBooking.workflowReminders.forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
- } catch (error) {
- log.error("Error while canceling scheduled workflow reminders", error);
- }
+ try {
+ // cancel workflow reminders from previous rescheduled booking
+ const deletions = originalRescheduledBooking.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ return Promise.resolve();
+ });
+ await Promise.all(deletions);
+ } catch (error) {
+ log.error("Error while canceling scheduled workflow reminders", error);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (originalRescheduledBooking?.uid) { | |
| try { | |
| // cancel workflow reminders from previous rescheduled booking | |
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); | |
| } catch (error) { | |
| log.error("Error while canceling scheduled workflow reminders", error); | |
| } | |
| if (originalRescheduledBooking?.uid) { | |
| try { | |
| // cancel workflow reminders from previous rescheduled booking | |
| const deletions = originalRescheduledBooking.workflowReminders.map((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| return deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| return Promise.resolve(); | |
| }); | |
| await Promise.all(deletions); | |
| } catch (error) { | |
| log.error("Error while canceling scheduled workflow reminders", error); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleNewBooking.ts` around lines 963 - 975,
The current loop over originalRescheduledBooking.workflowReminders calls async
functions deleteScheduledEmailReminder and deleteScheduledSMSReminder inside a
forEach without awaiting, so errors escape the try-catch; modify the block in
handleNewBooking where originalRescheduledBooking?.uid is checked to collect the
promises (e.g., map or push to an array) for each reminder depending on
reminder.method === WorkflowMethods.EMAIL or SMS, then await
Promise.all(promises) inside the existing try so any rejection is caught by the
catch; keep the existing guards out since the called functions already handle
null referenceId.
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | ||
| const remindersToCancel = await prisma.workflowReminder.findMany({ | ||
| where: { | ||
| cancelled: true, | ||
| scheduledDate: { | ||
| lte: dayjs().add(1, "hour").toISOString(), | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing filter for EMAIL method in cancellation query.
This handler is specifically for email reminders (scheduleEmailReminders.ts), but the query for remindersToCancel doesn't filter by method: WorkflowMethods.EMAIL. This could inadvertently pick up SMS reminders that are also marked as cancelled, leading to incorrect API calls (SendGrid for SMS reminders).
🐛 Proposed fix to add method filter
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
+ method: WorkflowMethods.EMAIL,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | |
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); | |
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | |
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| method: WorkflowMethods.EMAIL, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 43
- 51, The cancellation query in scheduleEmailReminders.ts is missing a method
filter and may return non-email reminders; update the
prisma.workflowReminder.findMany call that produces remindersToCancel to include
method: WorkflowMethods.EMAIL (alongside the existing cancelled and
scheduledDate filters) so only email reminders are selected; reference
WorkflowMethods.EMAIL, remindersToCancel, and prisma.workflowReminder.findMany
when making this change.
| try { | ||
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | ||
|
|
||
| for (const reminder of remindersToCancel) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); | ||
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
|
|
||
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
There was a problem hiding this comment.
Single failure aborts cancellation of remaining reminders.
The try/catch wraps the entire loop. If the SendGrid API call fails for one reminder (Line 57-64), the error is caught, logged, and the function continues—but all subsequent reminders in the loop are skipped. This could leave reminders in a stale cancelled: true state without being processed.
Consider wrapping the individual iteration in try/catch to ensure one failure doesn't prevent processing of other reminders:
🛡️ Proposed fix for resilient error handling
try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
for (const reminder of remindersToCancel) {
- await client.request({
- url: "/v3/user/scheduled_sends",
- method: "POST",
- body: {
- batch_id: reminder.referenceId,
- status: "cancel",
- },
- });
-
- const workflowReminderToDelete = prisma.workflowReminder.delete({
- where: {
- id: reminder.id,
- },
- });
-
- workflowRemindersToDelete.push(workflowReminderToDelete);
+ try {
+ await client.request({
+ url: "/v3/user/scheduled_sends",
+ method: "POST",
+ body: {
+ batch_id: reminder.referenceId,
+ status: "cancel",
+ },
+ });
+
+ const workflowReminderToDelete = prisma.workflowReminder.delete({
+ where: {
+ id: reminder.id,
+ },
+ });
+
+ workflowRemindersToDelete.push(workflowReminderToDelete);
+ } catch (error) {
+ console.log(`Error cancelling scheduled Email for reminder ${reminder.id}: ${error}`);
+ }
}
await Promise.all(workflowRemindersToDelete);
} catch (error) {
console.log(`Error cancelling scheduled Emails: ${error}`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| for (const reminder of remindersToCancel) { | |
| await client.request({ | |
| url: "/v3/user/scheduled_sends", | |
| method: "POST", | |
| body: { | |
| batch_id: reminder.referenceId, | |
| status: "cancel", | |
| }, | |
| }); | |
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| workflowRemindersToDelete.push(workflowReminderToDelete); | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } | |
| try { | |
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| for (const reminder of remindersToCancel) { | |
| try { | |
| await client.request({ | |
| url: "/v3/user/scheduled_sends", | |
| method: "POST", | |
| body: { | |
| batch_id: reminder.referenceId, | |
| status: "cancel", | |
| }, | |
| }); | |
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| workflowRemindersToDelete.push(workflowReminderToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Email for reminder ${reminder.id}: ${error}`); | |
| } | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 53
- 77, The current try/catch around the whole loop causes a single failure to
abort processing remaining reminders; change to per-iteration error isolation by
moving the try/catch inside the for loop so each reminder is processed
independently: for each reminder in remindersToCancel call client.request(...)
and prepare prisma.workflowReminder.delete(...) inside its own try block, push
successful deletes to workflowRemindersToDelete, and handle/log errors for that
specific reminder (include reminder.id or referenceId) without breaking the
loop, then await Promise.all(workflowRemindersToDelete) after the loop; symbols
to modify: remindersToCancel, client.request, prisma.workflowReminder.delete,
workflowRemindersToDelete.
| for (const reminder of remindersToCancel) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing null check for referenceId before SendGrid API call.
reminder.referenceId can be null (as per schema: referenceId String? @unique``). If a reminder was created but never scheduled with SendGrid (e.g., for far-future reminders), referenceId will be null. Passing `null` as `batch_id` to the SendGrid API will likely fail or cause unexpected behavior.
🐛 Proposed fix to guard against null referenceId
for (const reminder of remindersToCancel) {
+ if (!reminder.referenceId) {
+ // No SendGrid batch to cancel, just delete the reminder
+ workflowRemindersToDelete.push(
+ prisma.workflowReminder.delete({ where: { id: reminder.id } })
+ );
+ continue;
+ }
+
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 56
- 64, The loop over remindersToCancel calls client.request to POST to
"/v3/user/scheduled_sends" using reminder.referenceId as batch_id without
checking for null; add a guard inside the for (const reminder of
remindersToCancel) loop that skips (or logs and continues) when
reminder.referenceId is null/undefined before calling client.request so we never
send a null batch_id to the SendGrid scheduled_sends endpoint; reference the
reminder.referenceId check and the client.request(...) call in your change.
| <div className="mt-3 flex"> | ||
| <TextField | ||
| className=" border-r-transparent" | ||
| placeholder="Verification code" | ||
| value={verificationCode} | ||
| onChange={(e) => { | ||
| setVerificationCode(e.target.value); | ||
| }} | ||
| required | ||
| /> | ||
| <Button | ||
| color="secondary" | ||
| disabled={numberVerified || false} | ||
| className={classNames( | ||
| "-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ", | ||
| numberVerified ? "hidden" : "mt-3 sm:mt-0" | ||
| )} | ||
| onClick={() => | ||
| sendVerificationCodeMutation.mutate({ | ||
| className="-ml-[3px] rounded-tl-none rounded-bl-none " | ||
| disabled={verifyPhoneNumberMutation.isLoading} | ||
| onClick={() => { | ||
| verifyPhoneNumberMutation.mutate({ | ||
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | ||
| }) | ||
| }> | ||
| {t("send_code")} | ||
| code: verificationCode, | ||
| }); | ||
| }}> | ||
| Verify | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file to confirm the hardcoded strings
cat -n packages/features/ee/workflows/components/WorkflowStepContainer.tsx | sed -n '436,458p'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1357
🏁 Script executed:
# Check how useLocale is imported and used in the file
head -50 packages/features/ee/workflows/components/WorkflowStepContainer.tsx | grep -E "import|useLocale|const.*useLocale"Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 989
🏁 Script executed:
# Search for translation file patterns and existing translation keys
fd -e "json" -path "*locales*" -o -path "*translations*" -o -path "*i18n*" | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 258
🏁 Script executed:
# Look for how other strings are translated in this file
rg 't\(' packages/features/ee/workflows/components/WorkflowStepContainer.tsx -A 1 -B 1 | head -30Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 976
🏁 Script executed:
# Search for "verify" and "verification" keys in translation files
rg '"verify"|"verification"' --type jsonRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
🏁 Script executed:
# Find translation/locale files
find . -type f \( -name "*.json" -o -name "*.ts" -o -name "*.tsx" \) -path "*locale*" -o -path "*i18n*" -o -path "*lang*" | grep -E "locale|i18n|lang|translation" | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 992
🏁 Script executed:
# Search for where translations are stored
git ls-files | grep -iE "locale|i18n|translation|lang" | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 973
🏁 Script executed:
# Look for existing translation key patterns
rg 'verification_code|verified_successfully|wrong_code' --type json -B 2 -A 2 | head -40Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1424
🏁 Script executed:
# Search for "verify" in translation files more broadly
rg 'verify' --type json --ignore-case | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1727
🏁 Script executed:
# Search for all verification-related and verify-related keys in the English locale
rg '"verification|"verify' apps/web/public/static/locales/en/common.jsonRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 179
🏁 Script executed:
# Get context around the existing verification keys
cat apps/web/public/static/locales/en/common.json | grep -A 2 -B 2 'verification\|verify'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 616
🏁 Script executed:
# Check what button labels exist in the translation files
rg '": "' apps/web/public/static/locales/en/common.json | grep -E '"(button|action|label)' | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 182
🏁 Script executed:
# Look for similar UI element translations
rg '": "(Verify|Send|Submit|Cancel|Save)' apps/web/public/static/locales/en/common.json | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1166
🏁 Script executed:
# Check if there are any existing "verify" or "verification_code" keys in any translation file
rg '"verify":|"verification_code":' apps/web/public/static/locales/Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
Hardcoded strings break i18n support.
The placeholder text "Verification code" (line 439) and button text "Verify" (line 456) are hardcoded in English. These should use the t() translation function for consistency with the rest of the component.
Note: The proposed translation keys ("verify" and "verification_code") do not currently exist in the translation files. You'll need to add these keys to apps/web/public/static/locales/en/common.json and all other locale files, or reuse existing keys like "verification_code_sent" if semantically appropriate.
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx` around
lines 436 - 458, The placeholder "Verification code" on the TextField and the
button label "Verify" are hardcoded; update WorkflowStepContainer to call the
translation function t(...) for these strings (replace the TextField placeholder
and the Button children with t('verification_code') and t('verify')
respectively), keeping the existing verificationCode state and
verifyPhoneNumberMutation logic intact; then add matching keys
"verification_code" and "verify" to your locale JSON files (e.g., common.json)
for all supported locales or reuse appropriate existing keys.
| if (immediateDelete) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing database deletion when immediateDelete is true.
When immediateDelete is true, the function cancels the scheduled send with SendGrid but does not delete the reminder record from the database. This leaves orphaned records in the WorkflowReminder table.
Compare with deleteScheduledSMSReminder which always deletes the DB record, and the cron job in scheduleEmailReminders.ts which explicitly deletes after cancelling with SendGrid.
🐛 Proposed fix to add database deletion
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
+
+ await prisma.workflowReminder.delete({
+ where: {
+ id: reminderId,
+ },
+ });
+
return;
}🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/lib/reminders/emailReminderManager.ts` around
lines 213 - 223, When immediateDelete is true the code only cancels the SendGrid
scheduled send via client.request but never removes the WorkflowReminder DB
record; update the immediateDelete branch in emailReminderManager (where
client.request is called) to also delete the corresponding reminder record from
the database (mirror the behavior in deleteScheduledSMSReminder and the cron in
scheduleEmailReminders.ts) — locate the reminder by referenceId or the
function's reminder id parameter and call the same DB delete routine used
elsewhere so the WorkflowReminder row is removed after cancelling with SendGrid.
| //cancel workflow reminders of previous booking | ||
| bookingToReschedule.workflowReminders.forEach((reminder) => { | ||
| if (reminder.scheduled && reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| const reminderToDelete = prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); |
There was a problem hiding this comment.
Missing immediateDelete flag for email reminder cancellation.
Similar to the booking cancellation flow, deleteScheduledEmailReminder is called without the immediateDelete flag. For a reschedule operation, the previous booking's reminders should be cancelled immediately to prevent sending reminders for the old time slot.
The cron-based deferred cancellation (which runs hourly) may not process the cancellation in time if the rescheduled booking was close to its original start time.
🐛 Proposed fix to add immediateDelete flag
bookingToReschedule.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel workflow reminders of previous booking | |
| bookingToReschedule.workflowReminders.forEach((reminder) => { | |
| if (reminder.scheduled && reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| const reminderToDelete = prisma.workflowReminder.deleteMany({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| remindersToDelete.push(reminderToDelete); | |
| }); | |
| //cancel workflow reminders of previous booking | |
| bookingToReschedule.workflowReminders.forEach((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings.tsx` around lines 487 - 494, The
email reminder cancellation in the reschedule path omits the immediateDelete
flag so deleteScheduledEmailReminder(reminder.id, reminder.referenceId) should
be called with immediateDelete=true like in cancellation flows; update the loop
handling bookingToReschedule.workflowReminders to pass the third argument true
for email reminders (and leave deleteScheduledSMSReminder(reminder.id,
reminder.referenceId) unchanged) so previous booking emails are removed
immediately.
| //cancel workflow reminders of deleted workflow | ||
| scheduledReminders.forEach((reminder) => { | ||
| if (reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the definitions of the deletion functions
rg -n "export.*deleteScheduledEmailReminder|export.*deleteScheduledSMSReminder|function deleteScheduledEmailReminder|function deleteScheduledSMSReminder|const deleteScheduledEmailReminder|const deleteScheduledSMSReminder" packages -g '*.ts' -g '*.tsx' -A 2Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 761
🏁 Script executed:
# Also, let's look at the actual file and the lines mentioned in the review
head -n 230 packages/trpc/server/routers/viewer/workflows.tsx | tail -n 40Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1164
🏁 Script executed:
# Search more broadly for these functions
rg "deleteScheduledEmailReminder|deleteScheduledSMSReminder" packages -g '*.ts' -g '*.tsx' -lRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 429
🏁 Script executed:
# Check lines around 375-381
sed -n '360,390p' packages/trpc/server/routers/viewer/workflows.tsx | cat -nRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1259
🏁 Script executed:
# Check lines around 517-524
sed -n '500,540p' packages/trpc/server/routers/viewer/workflows.tsx | cat -nRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1785
🏁 Script executed:
# Check lines around 573-579
sed -n '555,590p' packages/trpc/server/routers/viewer/workflows.tsx | cat -nRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2033
Await reminder deletions; don't fire-and-forget.
Both deleteScheduledEmailReminder and deleteScheduledSMSReminder are async functions. The forEach loops calling them are not awaited, leaving deletions suspended and risking unhandled rejections.
For lines 211–218: referenceId is already guarded by the query filter (NOT: { referenceId: null }), so this block is safe from null issues.
For lines 375–381, 517–524, and 573–579: These blocks lack null checks and must filter out reminders where referenceId is null before attempting deletion.
Apply this pattern to all blocks:
- Filter out reminders where
referenceIdis null - Map to collect the deletion promises
- Await with
Promise.all()
Example fix
- remindersToDelete.flat().forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ await Promise.all(
+ remindersToDelete
+ .flat()
+ .filter((reminder) => reminder.referenceId)
+ .map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel workflow reminders of deleted workflow | |
| scheduledReminders.forEach((reminder) => { | |
| if (reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); | |
| //cancel workflow reminders of deleted workflow | |
| await Promise.all( | |
| scheduledReminders | |
| .filter((reminder) => reminder.referenceId) | |
| .map((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| return deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 211 - 218,
scheduledReminders are being deleted with fire-and-forget for
deleteScheduledEmailReminder and deleteScheduledSMSReminder; change each removal
block (the one iterating over scheduledReminders and the other similar blocks)
to first filter out reminders with null referenceId, map to the async deletion
calls (using WorkflowMethods.EMAIL/SMS to choose
deleteScheduledEmailReminder/deleteScheduledSMSReminder), collect the returned
promises, and await them with Promise.all() so deletions are awaited and
rejections handled.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Improvements